-
Notifications
You must be signed in to change notification settings - Fork 440
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Code improvements for ETW exporter #519
Conversation
…th existing ETW tooling.
…ueIterable: relax this restriction for now
…sistency), plus add converter from const char *
…etry/opentelemetry-cpp into maxgolov/etw_exporter
…try-cpp into maxgolov/etw_exporter
Codecov Report
@@ Coverage Diff @@
## main #519 +/- ##
==========================================
+ Coverage 94.51% 94.52% +0.01%
==========================================
Files 199 199
Lines 9136 9136
==========================================
+ Hits 8635 8636 +1
+ Misses 501 500 -1
|
@pyohannes - I addressed all of your comments, including stylistic comments. Re. API changes in a separate PR: instead I minimized the changes, and removed the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some additional comments for API related changes.
For lack of time I didn't give a deep review and test to the SDK related changes.
/** | ||
* @brief Null Span context that does not carry any information. | ||
*/ | ||
class NullSpanContext : public SpanContextKeyValueIterable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a long name, but let's use NullSpanContextKeyValueIterable
. To be consistent with SpanContextKeyValueIterable
. NullSpanContext
is ambiguous.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Anyway, to avoid an additional class, one could also do this:
using SpanContextKeyValueIterableInitializerList = SpanContextKeyValueIterableView<std::initializer_list<
std::pair<SpanContext,
std::initializer_list<std::pair<nostd::string_view, common::AttributeValue>>>>>;
And then define your additional StartSpan
method like this:
nostd::shared_ptr<Span> StartSpan(nostd::string_view name,
const common::KeyValueIterable &attributes,
const StartSpanOptions &options = {}) noexcept
{
return this->StartSpan(name, attributes, SpanContextKeyValueIterableInitializerList({}), options);
}
Just to avoid a clutter of types that we also need to document (and that are error-prone, e. g. return false
in case of success). We also have NullKeyValueIterable
, however, it's not used anywhere ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can refactor this in a separate (unrelated PR) by moving these into separate helper header or something. That would also enable NullTracerProvider
cases, unrelated to ETW per se. Right now it is more or less "internal" detail that I think we can rename / adjust before GA.
template <class T> | ||
class KeyValueIterableView final : public KeyValueIterable | ||
{ | ||
static_assert(detail::is_key_value_iterable<T>::value, "Must be a key-value iterable"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be safe. The actual contract is captured in the signature of ForEachKeyValue
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No problematic changes on API side. If not in this PR, we can remove NullSpanContext
(and maybe even NullKeyValuIterable
, which is unused) in a separate PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving changes to Attribute type enums + adding better HAVE_SPAN_BYTE support. +1 for diving through all that :)
Are you looking for bazel support for the various configuration of builds or are we ok just using CMake for that for now?
Thanks Josh. I'm good with CMake for now, but I'll take a look at Bazel right after. The way it stands - this part is kinda "self-sufficient" in a way that it's a header-only library, and the current customer who's gonna "preview" it - uses MSBuild / Visual Studio anyways (not even CMake nor Bazel). So I'll get back when I get a confirmation from them it works the way they wanted. Then will add the "proper" full range support in terms of our both official build systems. I will add E2E usage sample that covers the path from |
@maxgolov - While we merge this, we can also have a ticket to consolidate unresolved comments, to ensure they are handled after merge ? |
Yes, I'll take care of it. Logged two issues here:
|
Clean-up of ETW exporter
The following feature may be reusable by other components:
Properties
collection (container) with convenient initializer list operators and constructors to pass incoming event properties asstd::string
, vectors of integers/floats, string literals, etc. The way how it should be to be simple and look like JSON object initialization.Usage example
Note that the new class is fully compatible with
KeyValueIterable
, i.e. provides ABI-safe transform - should there be the need for ABI-safe interface. Note that across Visual Studio 2015-2017-2019, the ABI-safety for STL types is readily guaranteed on Windows. Thus, strictly speaking, unnecessary requirement that can be heavily optimized by avoiding the unnecessary tranformation.ETW channel -specific improvements
List of features added:
attributes
andlinks
TraceId
+SpanId
on individual eventsTests
ETW listener 'Test Framework' is written in C# - will be submited in contrib repo. This code also provides ability to Debug OpenTelemetry ETW exporter events in Visual Studio 2019 using built-in
Diagnostic Events
tab. Code is vendor-agnostic. I included examples of some other vendors using ETW. Those could be the candidates to adopt OpenTelemetry C++ SDK with ETW channel. Field names for ETW channel are also configurable, i.e. vendors may decide their own naming convention. It would make sense to normalize this with OTLP protocol names.Debugging Telemetry in Visual Studio 2019
Example screenshot that illustrates how test events show-up in Visual Studio 2019 with no additional tooling necessary:
This article explains more about Event Tracing for Windows and possible ways to export the data further:
https://docs.microsoft.com/en-us/azure/service-fabric/service-fabric-diagnostics-how-to-monitor-and-diagnose-services-locally
Possible flows will be explained further in contrib repo. Existing tools that allow to monitor and export ETW events to Elastic Search, for example: https://www.fireeye.com/blog/threat-research/2019/03/silketw-because-free-telemetry-is-free.html